The interesting part here is that in http1 finished doesn't seem to care whether the socket is closed or not, it only cares about whether end has been called or not. While in http2-compat it doesn't really care that much about end, instead if mostly cares about whether the socket is closed or not.

I think this discrepancy is also missing a test somewhere? @mcollina

EDIT: See, fab97ee

@jasnell can you help here?

The interesting part here is that in http1 finished doesn't seem to care whether the socket is closed or not, it only cares about whether end has been called or not. While in http2-compat it doesn't really care that much about end, instead if mostly cares about whether the socket is closed or not.

I think this discrepancy is also missing a test somewhere? @mcollina

I think this is just a difference that we need to fix.

I'm actually a bit unsure what happens if you call end on a closed connection in http1?

I'm actually a bit unsure what happens if you call end on a closed connection in http1?

if the stream is finished, nothing happens on multiple end() in HTTP1

If you are still unsure, have you trying preparing an example?

@mcollina

if the stream is finished, nothing happens on multiple end() in HTTP1

This is if you have called end() previously. But what if you call end for the first time after the socket has aborted.

Can you create an example to check this?

Sorry @mcollina, I just spotted the notification on this. I'll queue this one up and take a look hopefully later today.

@apapirovski care to expand on that?

The failing test looks wrong for me:

const server = createServer(mustCall((request, response) => {
    strictEqual(response.finished, true);
    response.writeHead(HTTP_STATUS_OK, { foo: 'bar' });
    response.end('data', mustCall());
  }));

It test that respone.finished is true before end() is called which is not how http/1 behaves.

@Trott: CI failed due to merge conflict. Please try again.

CI: https://ci.nodejs.org/job/node-test-pull-request/28619/

CI: https://ci.nodejs.org/job/node-test-pull-request/28665/

CI: https://ci.nodejs.org/job/node-test-pull-request/28724/

CI: https://ci.nodejs.org/job/node-test-pull-request/28848/

CI: https://ci.nodejs.org/job/node-test-pull-request/28936/

CI: https://ci.nodejs.org/job/node-test-pull-request/29120/

CI: https://ci.nodejs.org/job/node-test-pull-request/29133/

CI: https://ci.nodejs.org/job/node-test-pull-request/29134/

CI: https://ci.nodejs.org/job/node-test-pull-request/29165/

CI: https://ci.nodejs.org/job/node-test-pull-request/29177/

@nodejs/build I seem to be having problems with node-test-binary-arm-12+ in multiple PR's. Even those that just touch docs? #31805

Thanks @ronag, don't treat this as a blocker for your code, it's a Jenkins problem on our end.

@nodejs/build we're having a commit-ref propagation problem, in https://ci.nodejs.org/job/node-test-pull-request/29176/:

• https://ci.nodejs.org/job/node-cross-compile/27729/nodes=cross-compiler-armv7-gcc-6/console creates jenkins-node-test-commit-arm-fanned-e6a54b67290f8bab3de5b88504e531c20271f1aa-binary-pi1p/cc-armv7
• https://ci.nodejs.org/job/git-nodesource-update-reference/23898/console syncs jenkins-node-test-commit-arm-fanned-e6a54b67290f8bab3de5b88504e531c20271f1aa-binary-pi1p/cc-armv7 👍
• https://ci.nodejs.org/job/node-test-binary-arm-12+/4402/RUN_SUBSET=0,label=pi3-docker/console tries to pull jenkins-node-test-commit-arm-fanned-db24c2588175371c1ec2ec767541583ddf7fa0b1-binary-pi1p/cc-armv7 👎

anyone got time to look at this one? @joaocgreis I bet you'd have the most insight and could see the problem quickest if you're available.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2272/ (queued)

CI: https://ci.nodejs.org/job/node-test-pull-request/29218/ ✔️

Please do not use the "Resume build" when the PR changes. The "Resume build" only starts jobs that failed, so jobs that passed before will remain green, even if they would fail with the new changes. The "Resume build" feature is a source of problems, but it's the only way we have to reduce the impact of infra failures.

Please use "Rebuild" instead when there are any changes in the PR.

Both e6a54b6 and db24c25 were the head of #31805 at some point. Only the node-test-binary-arm-12+ job was started in https://ci.nodejs.org/job/node-test-pull-request/29176/. The other two were re-used from a previous PR because they passed, so the compile job never ran for the latest head.

Landed in 8ba7a2f